Skip to content

Conversation

AmeerArsala
Copy link

Here's the updated PR! This one fully has the changes you want!!

@lovell
Copy link
Owner

lovell commented Aug 26, 2024

Hi, thanks for the PR, did you see #25 and the need to use some form of safe-require-with-fallback approach?

@AmeerArsala
Copy link
Author

There! I did it. I tried opening a new PR but it took me here. added safeRequire with fallbacks.

@lovell
Copy link
Owner

lovell commented Aug 28, 2024

Thanks for the updates, as I mentioned in #25 I think the safeRequire function should return null when all options are exhausted (rather than throw), and consumers need to guard against this. Are you able to update the logic to match this behaviour?

Please can you also remove the formatting changes to ensure semistandard will pass, as well as keeping the diff cleaner.

@AmeerArsala
Copy link
Author

Done! My bad, I didn't realize my editor automatically made the formatting changes. It returns null now

@AmeerArsala
Copy link
Author

Any update on this?

@lovell
Copy link
Owner

lovell commented Sep 4, 2024

Given that safeRequire() can return null, consumers need to guard against this, e.g. childProcess might be null. Are you able to update the logic to match this behaviour?

@AmeerArsala
Copy link
Author

Alright, updated! Sorry for the long wait, I just saw this

@AmeerArsala
Copy link
Author

Ok so from my inspection I saw that the for loop I made was the issue. I changed it from being a foreach loop to just a regular for loop in case some versions of Node don't play nice with it in this context.

@eigilsagafos
Copy link

eigilsagafos commented Sep 25, 2025

@lovell Any plans to merge this? Or would you be open to another PR with a different approach? I'm facing similar issues in using Bun build with the browser as target.

@lovell
Copy link
Owner

lovell commented Sep 25, 2025

@eigilsagafos Please can you open a new issue about this with steps to reproduce as there may be alternatives that could help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants